-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
No more are you sure boxes #4101
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I really like this so far, but I think it'll be easiest to just add the setting in profiles.json
right now, and not add the checkbox in this PR.
Right now, we're only really writing out the profiles.json
file when dynamic profiles are added, and when that happens, we're surgically modifying the profiles.json
file to insert the new profiles - we're not re-serializing the settings. Unfortunately, I'm not sure that surgical insertion code could be easily re-used for this scenario. It might be tricky to add similar code to enable writing out an update of a single setting like this. If you're really interested, you could look at CascadiaSettings::_AppendDynamicProfilesToUserSettings
and CascadiaSettings::_PrependSchemaDirective
for samples, but HERE BE DRAGONS.
I'd rather just get the setting added now, and figure out the tricky part later IMO.
Remove checkbox from confirm dialog.
Ok, so I also removed the checkbox (and related event handlers) as well. Though I would like to revisit doing that later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also make sure you update the following files with the new setting:
I think the setting key is ok too, but I wanna quickly poll @cinnamon-msft for her thoughts just to keep her in the loop 😊
Apparently trying to fix merge conflicts using GitHub's web-based conflict resolution thing screws up the formatting.
(ok so this time it shouldn't screw with the formatting)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it. Thanks!
I dig this :) |
Hello @miniksa! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
🎉 Handy links: |
Summary of the Pull Request
So this PR adds a profile setting called "confirmCloseAllTabs", that allows one to enable or disable the "Do you want close all tabs?" dialog that appears when you close a window with multiple open tabs. It current defaults to "true". Also adds a checkbox to that dialog that also sets "confirmCloseAllTabs"
References
PR Checklist
Detailed Description of the Pull Request / Additional comments
I added a checkbox to the close dialog to set this setting, but I'm not sure how to best go about actually changing the setting from code; am open to suggestions, as to how it should be done, or if I should also just remove it and stick with the profile setting.
Validation Steps Performed